-
Notifications
You must be signed in to change notification settings - Fork 130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
UI Improvement and Features #705
Conversation
A lot of this looks really awesome, thanks! Just gonna have to let this sit here for a little bit though as I have another PR that has a bunch of stuff I need to get merged first before I can work on or merge anything else. I don't think it will break too much here though. Once I get that stuff finished, tested, and merged I can come back to review this. Sorry for the delay. |
Ok that PR is merged so you can take a look at those merge conflicts. I haven't reviewed in depth but from an overview perspective there's a few things I have feedback on.
Overall, the rest looks pretty great. |
Ill take a look at the merge conflicts and make the changes you suggested. Thanks for the feedback! |
I have merged and made the improvements you mentioned earlier. I have also fixed the app icon not working on Linux systems and added a path/auto search bars on the Project Browser. I'm also loving the new swerve modules visualization! |
Looking good. Can you add tests for the new stuff you added? Most things you touched should already be covered, but testing the search bars, the extra info displays in the path tree, the position text field for event markers/rotation targets, etc. would be great. There's lots of examples in the tests already there so it should hopefully be easy to figure out but if you need any help let me know. It also looks like a few tests are failing so you'll need to look into why they're not working. |
Sounds good, I will get to work on those tests. |
The mocks need to be generated using |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #705 +/- ##
==========================================
- Coverage 84.72% 84.38% -0.34%
==========================================
Files 86 88 +2
Lines 7534 7890 +356
==========================================
+ Hits 6383 6658 +275
- Misses 1151 1232 +81 ☔ View full report in Codecov by Sentry. |
It looks like a lot of coverage is missing for some of the tests. From looking at the coverage report it looks like most of it was previously covered before this PR but now isn't. Perhaps you deleted some tests and never added them back? telemetry_page.dart specifically looks like it lost a lot of coverage. Try seeing if you can get those tests back and we'll see what the project coverage looks like after that. I usually try to aim for not letting the total project coverage decrease. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need to actually run the build and see if I have any feedback for how it looks/works, but from a code perspective it looks pretty good besides a few minor suggestions.
I will go ahead and take a look at your suggestions and codecov's report. Hopefully we can get that coverage back up. |
A test slipped one of my checks. Ill get to work on it and try to improve coverage. |
Alright i made some improvements to the tests on the path tree, telemetry page, and waypoints tree. |
So I think a couple things are still hurting your coverage:
Addressing those things should probably be enough to get the coverage back up to a good level. |
Okay, i updated the telemetry page test and my local test coverage says its pretty good. I've also fixed the showDetails flag. |
Ok so the fix I gave you for painting path details didn't work because in tests it never gets a trajectory generated because that happens asynchronously. The real fix here is to add that default setting to true in split_choreo_path_editor_test since that will actually draw trajectories. I've verified that this actually does work. Also just noticed a few other coverage related things:
Sorry for making you chase coverage so much lol. But having things properly covered has saved me from breaking things so many times so I think its very important. |
Its alright lol |
I've been very busy this past week so sorry for the delay. Most of the changes are almost implemented and i will take a look at the PR mentioned. |
No rush. Just as a heads up I'm currently working on a PR to update the Writerside documentation for all the changes I made but haven't documented yet. After that's merged you should update the docs for your changes in this PR. All you should have to do is update a few screenshots and do quick descriptions of additions like the new editor settings. Writerside is a free Jetbrains app or IntelliJ plugin, but the changes you'd need to make are so minor you can probably just edit the raw Markdown/replace existing images. The documentation updates can also be done in a future PR if you'd prefer to get this merged a bit quicker. I don't really care either way as long as the docs get updated before beta which is still a ways out. |
I would prefer to use a separate PR for the documentation, thanks! |
Just checking in to see how things are going with this. I have a branch ready for #690 which touches most files in the project so I want to get this merged before that if you think it'll be ready in the next few days so that I don't cause a million different merge conflicts for you. Again, no rush, just wondering if its worth holding off. |
It should be ready tonight. I am working on the current conflicts. Thanks for your patience |
Okay I have merged everything and ran LCOV with 84.4% of the project being covered. I looked into the issue of #723 and could not replicate it. |
Ok. I'll try to keep a look out for that issue. Thanks for all the work on this. |
It only appears to happen when "Recent" is the sort by method. I tested again and could not have the issue occur when sorted Alphabetically. It shouldn't be a huge deal, even if it's an issue. |
@mjansen4857 Thanks for all your help with this PR, I learned a lot! |
No problem. Just a heads up I made a couple minor fixes to things I noticed after merging in #776 Main things were some cards were darker than others and the robot details would rotate with the robot. |
UI Improvement and Features
This PR adds significant UI changes and several new features to improve the user experience and functionality.
New Features:
UI Improvements:
As this is my first public pull request, I would greatly appreciate feedback. I've also included several before/after images demonstrating the UI changes.
Before & After Images
Path Editor
Sidebar
Telemetry
Grid & Robot Details
Waypoints Tree
Rotation Target Tree